Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test that the coordinator broadcasts spend txs #361

Merged

Conversation

danielabrozzoni
Copy link
Collaborator

@danielabrozzoni danielabrozzoni commented Feb 1, 2022

@danielabrozzoni danielabrozzoni force-pushed the 20220201_coord_broadcast_test branch 2 times, most recently from 8cafa6a to cf12dba Compare February 7, 2022 09:58
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits around bad copy pasta of comments, other than that LGTM

tests/test_spend.py Outdated Show resolved Hide resolved
tests/test_spend.py Outdated Show resolved Hide resolved
tests/test_spend.py Outdated Show resolved Hide resolved
@danielabrozzoni
Copy link
Collaborator Author

danielabrozzoni commented Feb 8, 2022

Fixed, and rebased on top of #365 to have the coord update commit :)

Edit: actually I'll just close #365 and keep this one instead

I'm not really sure why we would test with a deployment that
big (I'm fairly convinced it was just bad copypasta from another
test). This should reduce the amount of timeouts in CI.
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 5075f23

@@ -386,8 +386,8 @@ def test_spends_conflicting(revault_network, bitcoind):
@pytest.mark.skipif(not POSTGRES_IS_SETUP, reason="Needs Postgres for servers db")
def test_spend_threshold(revault_network, bitcoind, executor):
CSV = 20
managers_threshold = 3
revault_network.deploy(17, 8, csv=CSV, managers_threshold=managers_threshold)
managers_threshold = 2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment about the commit message, not this line.

I'm not really sure why we would test with a deployment that
big (I'm fairly convinced it was just bad copypasta from another
test). This should reduce the amount of timeouts in CI.

My rationale was testing larger setups. It should be an explicitly dedicated test, instead so ACK for decreasing it.

@darosior darosior merged commit 3ab3c6a into revault:master Feb 9, 2022
@danielabrozzoni danielabrozzoni deleted the 20220201_coord_broadcast_test branch February 28, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants